【iOS】Fixes ellipsis carries background from trimmed text by zhongwuzw · Pull Request #39408 · facebook/react-native

zhongwuzw · 2023-09-12T10:33:02Z

Summary:

Changelog:

[IOS] [FIXED] - Fixes ellipsis carries background from trimmed text

Test Plan:

Demo code:

<Text numberOfLines={1} ellipsizeMode={'tail'}>Long text <Text style={{backgroundColor: 'red'}}>Nested text</Text></Text>

Before fix:

After fix:

javache · 2023-09-12T10:58:50Z

I don't think we want to mutate _textStorage in the draw path. Would it be possible to do so in the layout path? Also, could you verify this behaviour in the new architecture too?

analysis-bot · 2023-09-12T11:01:28Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	21,284,129	+49,399
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	24,481,163	+49,241
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 778fcec
Branch: main

zhongwuzw · 2023-09-12T11:27:47Z

I don't think we want to mutate _textStorage in the draw path. Would it be possible to do so in the layout path? Also, could you verify this behaviour in the new architecture too?

@javache Thanks for the review, I have no idea how to do it in the layout path, my thought is we can mutate the textStorage before drawing and restore the textStorage which we mutated after drawing.

And I tested in new arch and it has the same issue.

javache · 2023-09-13T14:25:33Z

I would look at making this change in RCTTextShadowView and RCTTextLayoutManager (for the new architecture)

zhongwuzw · 2023-10-08T03:29:43Z

@javache Friendly ping, thank you for any feedback :)

javache

Please see my previous feedback about moving this logic on the background thread by using RCTTextShadowView/RCTTextLayoutManager. Please also ensure this is compatible with the new architecture.

This reverts commit 96a31c2.

zhongwuzw · 2023-10-17T06:23:09Z

        layoutContext,
        layoutConstraints,
-        nullptr);
+        hostTextStorage_);


Seems Fabric uses different textStorage in measure and draw path. We should share it.

…/fix_ellipsis_background

github-actions · 2024-05-08T05:00:54Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

zhongwuzw · 2024-05-10T09:16:45Z

@javache Seems we can't put it into background thread In new arch, because we use separate textstorage for measure and draw path, shared of textstorage was removed in #43914.

zhongwuzw · 2024-07-09T03:25:26Z

any one can help to review? cc. @sammy-SC @javache

javache · 2024-07-09T09:44:50Z

Thanks for investigating this. The changes look reasonable, but can we add gating here so we only execute this if numberOfLines is set?

…is_background

zhongwuzw · 2024-07-09T10:40:15Z

@javache Done :)

facebook-github-bot · 2024-07-09T12:21:09Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2024-07-09T13:06:07Z

Hi @zhongwuzw, I know that you are fixing a specific issue, but there could be other style properties that we want to remove from the ellipsis.

I think that the ellispis should keep the same style of the character that precedes it, not to have weird artifacts on the screen.
what do you think?

zhongwuzw · 2024-07-10T09:07:58Z

@cipolleschi Hi, I just keep the same color related attributes as the character that precedes it. Other attributes like fontSize would impact the layout of the text.

facebook-github-bot · 2024-07-10T11:15:05Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2024-07-10T12:30:54Z

This is failing internal builds with

packages/react-native/Libraries/Text/Text/RCTTextShadowView.mm:262:64: error: declaration shadows a local variable [-Werror,-Wshadow]
                                     NSTextContainer *_Nonnull textContainer,

packages/react-native/Libraries/Text/Text/RCTTextShadowView.mm:263:46: error: declaration shadows a local variable [-Werror,-Wshadow]
                                     NSRange glyphRange,

zhongwuzw · 2024-07-10T13:42:00Z

@javache done 😀

facebook-github-bot · 2024-07-11T11:26:45Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-07-11T15:50:04Z

@javache merged this pull request in fcb6cdc.

github-actions · 2024-07-11T15:50:08Z

This pull request was successfully merged by @zhongwuzw in fcb6cdc.

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2024-07-12T08:50:52Z

This pull request has been reverted by b1c5432.

…5412) Summary: Reland #39408 . Try to fix the crash #37926 (comment). cc. javache I changed the range from glyph to character because all attributes related APIs are character range. ## Changelog: [IOS] [FIXED] - Fixes ellipsis carries background from trimmed text Pull Request resolved: #45412 Test Plan: #37926 . Reviewed By: cipolleschi Differential Revision: D59681679 Pulled By: javache fbshipit-source-id: de4cb73e0304b8c0b0a40f4f6838b2c679747009

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 12, 2023

javache requested changes Oct 9, 2023

View reviewed changes

zhongwuzw added 3 commits October 17, 2023 10:01

【iOS】Fixes ellipsis carries background from trimmed text

96a31c2

Revert "【iOS】Fixes ellipsis carries background from trimmed text"

4817a95

This reverts commit 96a31c2.

Support old and Fabric arch

aadbf6f

zhongwuzw force-pushed the features/fix_ellipsis_background branch from 08df8de to aadbf6f Compare October 17, 2023 02:17

zhongwuzw requested a review from javache October 17, 2023 06:18

zhongwuzw commented Oct 17, 2023

View reviewed changes

Merge commit 'd7af27dd7da529eb3feda12c45eefe35f9a7f79f' into features…

8217fb8

…/fix_ellipsis_background

github-actions Bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label May 8, 2024

zhongwuzw added 2 commits May 10, 2024 16:30

Merge master

b8cfea9

Fix for new arch

92f860a

github-actions Bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label May 11, 2024

zhongwuzw added 2 commits July 9, 2024 17:53

Merge branch 'features/fix_bridgeless_block' into features/fix_ellips…

e84336a

…is_background

Add maximunNumberOfLines gate

c3afd09

zhongwuzw added 2 commits July 10, 2024 15:55

Keep the same style of the character that precedes it

20ccff6

Simply remove attributes operation

80cd8e0

zhongwuzw added 2 commits July 10, 2024 17:02

Only remove color attributes

19e3ef3

optimize code

41a1132

fix shallow variable

3c536f7

facebook-github-bot closed this in fcb6cdc Jul 11, 2024

facebook-github-bot added the Merged This PR has been merged. label Jul 11, 2024

facebook-github-bot added the Reverted label Jul 12, 2024

zhongwuzw mentioned this pull request Jul 12, 2024

Reland: [iOS] Fixes ellipsis carries background from trimmed text #45412

Closed

Conversation

zhongwuzw commented Sep 12, 2023

Summary:

Changelog:

Test Plan:

Uh oh!

javache commented Sep 12, 2023

Uh oh!

analysis-bot commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongwuzw commented Sep 12, 2023

Uh oh!

javache commented Sep 13, 2023

Uh oh!

zhongwuzw commented Oct 8, 2023

Uh oh!

javache left a comment

Choose a reason for hiding this comment

Uh oh!

zhongwuzw Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

zhongwuzw commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongwuzw commented Jul 9, 2024

Uh oh!

javache commented Jul 9, 2024

Uh oh!

zhongwuzw commented Jul 9, 2024

Uh oh!

facebook-github-bot commented Jul 9, 2024

Uh oh!

cipolleschi commented Jul 9, 2024

Uh oh!

zhongwuzw commented Jul 10, 2024

Uh oh!

facebook-github-bot commented Jul 10, 2024

Uh oh!

javache commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongwuzw commented Jul 10, 2024

Uh oh!

facebook-github-bot commented Jul 11, 2024

Uh oh!

facebook-github-bot commented Jul 11, 2024

Uh oh!

github-actions Bot commented Jul 11, 2024

Uh oh!

facebook-github-bot commented Jul 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

analysis-bot commented Sep 12, 2023 •

edited

Loading

zhongwuzw commented May 10, 2024 •

edited

Loading

javache commented Jul 10, 2024 •

edited

Loading